From 3cac5a941b10c317c2fd3c953d2fe4905ff39e31 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 17 Aug 2026 19:29:35 -0400 Subject: [PATCH] static-delta: Drop unreliable usize-based margin heuristic The per-object margin formula added in the previous commit tries to bound the gap between a delta part's declared usize (sum of reconstructed object sizes) and its actual decompressed payload size by scaling with the number of objects in the part. That's the wrong axis: the biggest source of that gap is the bsdiff patch stream for bsdiff'd objects, which scales with object *content* size, not object *count*. A part with just one or two large bsdiff'd objects (e.g. a big shared library that changed significantly between releases) blows right past the margin, causing legitimate deltas to be rejected at apply time. This is the root cause of https://github.com/ostreedev/ostree/issues/3635. Just drop it, we'll work to add a more reliable limit later. Assisted-by: https://github.com/cgwalters/cgwalters#llms Signed-off-by: Colin Walters --- src/libostree/ostree-repo-pull.c | 15 ++-- src/libostree/ostree-repo-static-delta-core.c | 63 ++------------- .../ostree-repo-static-delta-private.h | 79 +++---------------- 3 files changed, 25 insertions(+), 132 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 5f729e68..63277f2a 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -1270,12 +1270,9 @@ static_deltapart_fetch_on_complete (GObject *object, GAsyncResult *result, gpoin /* Transfer ownership of the fd */ in = g_unix_input_stream_new (g_steal_fd (&tmpf.fd), TRUE); - guint32 n_objects - = (guint32)(g_variant_get_size (fetch_data->objects) / OSTREE_STATIC_DELTA_OBJTYPE_CSUM_LEN); /* TODO - make async */ - if (!_ostree_static_delta_part_open (in, NULL, 0, fetch_data->expected_checksum, - fetch_data->usize, n_objects, &part, pull_data->cancellable, - error)) + if (!_ostree_static_delta_part_open (in, NULL, 0, fetch_data->expected_checksum, &part, + pull_data->cancellable, error)) goto out; _ostree_static_delta_part_execute_async (pull_data->repo, fetch_data->objects, part, @@ -2214,12 +2211,10 @@ process_one_static_delta (OtPullData *pull_data, const char *from_revision, cons g_autoptr (GInputStream) memin = g_memory_input_stream_new_from_bytes (inline_part_bytes); g_autoptr (GVariant) inline_delta_part = NULL; - guint32 n_objects - = (guint32)(g_variant_get_size (objects) / OSTREE_STATIC_DELTA_OBJTYPE_CSUM_LEN); /* For inline parts we are relying on per-commit GPG, so don't bother checksumming. */ - if (!_ostree_static_delta_part_open ( - memin, inline_part_bytes, OSTREE_STATIC_DELTA_OPEN_FLAGS_SKIP_CHECKSUM, NULL, - usize, n_objects, &inline_delta_part, cancellable, error)) + if (!_ostree_static_delta_part_open (memin, inline_part_bytes, + OSTREE_STATIC_DELTA_OPEN_FLAGS_SKIP_CHECKSUM, NULL, + &inline_delta_part, cancellable, error)) { fetch_static_delta_data_free (fetch_data); return FALSE; diff --git a/src/libostree/ostree-repo-static-delta-core.c b/src/libostree/ostree-repo-static-delta-core.c index fee27de0..abcca819 100644 --- a/src/libostree/ostree-repo-static-delta-core.c +++ b/src/libostree/ostree-repo-static-delta-core.c @@ -585,10 +585,8 @@ ostree_repo_static_delta_execute_offline_with_signature (OstreeRepo *self, GFile */ delta_open_flags |= OSTREE_STATIC_DELTA_OPEN_FLAGS_SKIP_CHECKSUM; - guint32 n_objects - = (guint32)(g_variant_get_size (objects) / OSTREE_STATIC_DELTA_OBJTYPE_CSUM_LEN); if (!_ostree_static_delta_part_open (part_in, inline_part_bytes, delta_open_flags, NULL, - usize, n_objects, &part, cancellable, error)) + &part, cancellable, error)) return FALSE; } else @@ -600,10 +598,8 @@ ostree_repo_static_delta_execute_offline_with_signature (OstreeRepo *self, GFile part_in = g_unix_input_stream_new (part_fd, FALSE); - guint32 n_objects - = (guint32)(g_variant_get_size (objects) / OSTREE_STATIC_DELTA_OBJTYPE_CSUM_LEN); - if (!_ostree_static_delta_part_open (part_in, NULL, delta_open_flags, checksum, usize, - n_objects, &part, cancellable, error)) + if (!_ostree_static_delta_part_open (part_in, NULL, delta_open_flags, checksum, &part, + cancellable, error)) return FALSE; } @@ -637,64 +633,19 @@ ostree_repo_static_delta_execute_offline (OstreeRepo *self, GFile *dir_or_file, self, dir_or_file, NULL, skip_validation, cancellable, error); } -/* Compute how much larger than the declared usize a part's decompressed - * payload is allowed to be. See the constants in - * ostree-repo-static-delta-private.h for the derivation of each term; - * all arithmetic saturates to G_MAXUINT64 on overflow rather than - * wrapping, since expected_usize comes from the (not yet fully - * validated) delta part header. - */ -static guint64 -_ostree_static_delta_compute_part_margin (guint64 expected_usize, guint32 expected_n_objects) -{ - guint64 margin = OSTREE_STATIC_DELTA_PART_FIXED_OVERHEAD_BYTES; - - guint64 per_object_overhead; - if (!g_uint64_checked_mul (&per_object_overhead, (guint64)expected_n_objects, - OSTREE_STATIC_DELTA_PART_OP_OVERHEAD_PER_OBJECT_BYTES - + OSTREE_STATIC_DELTA_PART_XATTR_ALLOWANCE_PER_OBJECT_BYTES) - || !g_uint64_checked_add (&margin, margin, per_object_overhead)) - return G_MAXUINT64; - - const guint64 rollsum_overhead - = expected_usize / OSTREE_STATIC_DELTA_PART_ROLLSUM_OVERHEAD_DIVISOR; - if (!g_uint64_checked_add (&margin, margin, rollsum_overhead)) - return G_MAXUINT64; - - return margin; -} - gboolean _ostree_static_delta_part_open (GInputStream *part_in, GBytes *inline_part_bytes, OstreeStaticDeltaOpenFlags flags, const char *expected_checksum, - guint64 expected_usize, guint32 expected_n_objects, GVariant **out_part, GCancellable *cancellable, GError **error) { const gboolean trusted = (flags & OSTREE_STATIC_DELTA_OPEN_FLAGS_VARIANT_TRUSTED) > 0; const gboolean skip_checksum = (flags & OSTREE_STATIC_DELTA_OPEN_FLAGS_SKIP_CHECKSUM) > 0; - /* Use the declared usize from the delta header as the decompression limit - * when available, capped to the hard maximum. If the caller passes 0 - * (e.g. the show/dump path) fall back to the hard cap alone. - * - * The declared usize only covers the final on-disk size of the objects - * a part produces, not the part payload itself (which additionally - * contains the mode/xattr tables and operations bytecode). Add a - * margin on top of usize, derived from the number of objects in the - * part, so legitimate parts aren't rejected; see - * _ostree_static_delta_compute_part_margin(). + /* Decompression-bomb defense: the flat hard cap. A tighter, + * exact-size-derived limit may be layered on top of this by callers + * that have that information; see OSTREE_STATIC_DELTA_PART_MAX_USIZE_BYTES. */ guint64 max_part_usize = OSTREE_STATIC_DELTA_PART_MAX_USIZE_BYTES; - if (expected_usize > 0) - { - const guint64 margin - = _ostree_static_delta_compute_part_margin (expected_usize, expected_n_objects); - guint64 bounded_usize; - if (!g_uint64_checked_add (&bounded_usize, expected_usize, margin)) - bounded_usize = G_MAXUINT64; - if (bounded_usize < max_part_usize) - max_part_usize = bounded_usize; - } /* We either take a fd or a GBytes reference */ g_return_val_if_fail (G_IS_FILE_DESCRIPTOR_BASED (part_in) || inline_part_bytes != NULL, FALSE); @@ -820,7 +771,7 @@ show_one_part (OstreeRepo *self, gboolean swap_endian, const char *from, const c g_autoptr (GVariant) part = NULL; if (!_ostree_static_delta_part_open (part_in, NULL, OSTREE_STATIC_DELTA_OPEN_FLAGS_SKIP_CHECKSUM, - NULL, 0, 0, &part, cancellable, error)) + NULL, &part, cancellable, error)) return FALSE; { diff --git a/src/libostree/ostree-repo-static-delta-private.h b/src/libostree/ostree-repo-static-delta-private.h index 7d325448..2fd5135a 100644 --- a/src/libostree/ostree-repo-static-delta-private.h +++ b/src/libostree/ostree-repo-static-delta-private.h @@ -32,73 +32,21 @@ G_BEGIN_DECLS * provides ~16x headroom over the default, which is generous enough to * accommodate large custom --max-chunk-size values while still rejecting * decompression bombs that would expand to gigabytes. + * + * This is also the sole bound applied to deltas that don't carry the + * exact-payload-size metadata (see + * OSTREE_STATIC_DELTA_PART_PAYLOAD_SIZES_KEY below): the declared "usize" + * in a delta part header only accounts for the final on-disk size of the + * objects the part will produce, not the mode/xattr tables, opcode + * bytecode, or raw payload data (including, for bsdiff'd objects, the + * entire patch stream) that also make up the part's decompressed + * payload. There is no way to derive a tight, correct bound from usize + * alone -- a part with a single large bsdiff'd object can have a + * decompressed payload many times its usize -- so deltas lacking the + * exact size just fall back to this flat cap. */ #define OSTREE_STATIC_DELTA_PART_MAX_USIZE_BYTES (512ULL * 1024ULL * 1024ULL) -/* The declared "usize" in a delta part header only accounts for the final - * on-disk size of the objects the part will produce; the part payload - * that actually gets decompressed is larger. The constants below bound - * that difference so a decompression limit can be derived from usize - * without either false-positiving on legitimate parts or degenerating - * into a check that never rejects anything (see - * _ostree_static_delta_compute_part_margin() in - * ostree-repo-static-delta-core.c for how they're combined). Each term - * is sized from the actual on-disk formats in - * ostree-repo-static-delta-compilation.c and ostree-varint.c rather than - * from a single round guess, so the resulting margin stays proportionate - * as the number of objects in a part grows. - */ - -/* Flat per-part overhead: GVariant framing for the part's outer tuple and - * the operations/payload byte arrays. A few KiB is generous here; this - * doesn't scale with content. - */ -#define OSTREE_STATIC_DELTA_PART_FIXED_OVERHEAD_BYTES (4ULL * 1024ULL) - -/* Per-object operations overhead: each object contributes a mode table - * entry ("(uuu)", 12 bytes, deduplicated but bounded per-object in the - * worst case) plus opcode bytecode. _ostree_write_varuint64() emits at - * most 10 bytes, and the largest per-object opcode sequence is the - * bsdiff path (SET_READ_SOURCE, OPEN, BSPATCH, CLOSE, UNSET_READ_SOURCE: - * 5 opcodes + 6 varints = 65 bytes) plus its embedded 32-byte source - * checksum, which isn't counted in usize at all. 109 bytes worst case; - * round up generously. - */ -#define OSTREE_STATIC_DELTA_PART_OP_OVERHEAD_PER_OBJECT_BYTES 256ULL - -/* Per-object xattr allowance: xattrs are written into the payload in full - * and aren't reflected in usize either. There's no way to derive a true - * worst-case bound for this from filesystem limits: ext4 caps total - * attribute bytes per inode at ~4 KiB (one external block plus a little - * in-inode space), but that bound doesn't hold in general -- XFS and - * Btrfs impose no total per-inode limit, and even ext4 with the - * (non-default) ea_inode feature can push individual values up to - * XATTR_SIZE_MAX (64 KiB) each across its ~100-250 max entries. A file - * could in principle carry many such values on some filesystem; fully - * covering that here would require a per-object allowance in the tens of - * MiB, which for parts with more than a handful of objects would swamp - * this margin and degenerate the check back into "always equal to the - * hard cap" -- the exact failure mode we moved away from a flat - * 1 MiB/object margin to avoid. So use a single XATTR_SIZE_MAX (64 KiB) - * as a generous but pragmatic allowance: real xattr sets (SELinux label, - * capabilities, ACLs, IMA/EVM signatures) total well under 2 KiB in - * practice, so this covers legitimate content with 30x+ headroom to - * spare. Pathological xattr counts beyond that are left to - * OSTREE_STATIC_DELTA_PART_MAX_USIZE_BYTES, the same hard-cap backstop - * that bounds this whole margin. - */ -#define OSTREE_STATIC_DELTA_PART_XATTR_ALLOWANCE_PER_OBJECT_BYTES (64ULL * 1024ULL) - -/* Rollsum overhead divisor: rollsum (bsdiff-like binary delta against a - * similar file) emits a WRITE op per matched/unmatched chunk, and chunk - * boundaries come from bupsplit's content-defined chunking, which - * averages BUP_BLOBSIZE (8 KiB) per chunk. At up to ~64 bytes of opcode - * overhead per chunk, that's an expected overhead of roughly usize/128; - * dividing by 32 instead bakes in a further 4x safety margin for content - * that chunks more finely than average. - */ -#define OSTREE_STATIC_DELTA_PART_ROLLSUM_OVERHEAD_DIVISOR 32ULL - /* 1 byte for object type, 32 bytes for checksum */ #define OSTREE_STATIC_DELTA_OBJTYPE_CSUM_LEN 33 @@ -217,8 +165,7 @@ typedef enum gboolean _ostree_static_delta_part_open (GInputStream *part_in, GBytes *inline_part_bytes, OstreeStaticDeltaOpenFlags flags, - const char *expected_checksum, guint64 expected_usize, - guint32 expected_n_objects, GVariant **out_part, + const char *expected_checksum, GVariant **out_part, GCancellable *cancellable, GError **error); typedef struct -- 2.39.5